-
Notifications
You must be signed in to change notification settings - Fork 2.5k
fix: surface MCP server errors to UI with toast notifications #7640
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Updated showErrorMessage() to display errors via vscode.window.showErrorMessage() - Added UI error notifications for transport errors (stdio, SSE, streamable-http) - Ensures users get visible feedback when MCP server connections fail - Maintains console logging for debugging while adding user-facing notifications Fixes #7639
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing my own code is like debugging in a mirror - everything looks backwards and I still can't find the problem.
| const fullMessage = `${message}: ${errorMessage}` | ||
| console.error(fullMessage) | ||
| vscode.window.showErrorMessage(fullMessage) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| transport.onerror = async (error) => { | ||
| console.error(`Transport error for "${name}":`, error) | ||
| const errorMessage = error instanceof Error ? error.message : `${error}` | ||
| const fullMessage = `Transport error for MCP server "${name}": ${errorMessage}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message formatting is inconsistent across transport types. Notice that stdio doesn't include "(stdio)" suffix while SSE includes "(SSE)" and streamable-http includes "(streamable-http)". Could we standardize this for consistency?
| console.error(`Transport error for "${name}":`, error) | ||
| const errorMessage = error instanceof Error ? error.message : `${error}` | ||
| const fullMessage = `Transport error for MCP server "${name}": ${errorMessage}` | ||
| console.error(fullMessage) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I notice we're calling console.error twice for the same error - once here in the transport error handler and once inside showErrorMessage (line 262). This results in duplicate console logging. Should we remove the console.error from these transport handlers since showErrorMessage already handles it?
| console.error(`Transport error for "${name}" (streamable-http):`, error) | ||
| const errorMessage = error instanceof Error ? error.message : `${error}` | ||
| const fullMessage = `Transport error for MCP server "${name}" (streamable-http): ${errorMessage}` | ||
| console.error(fullMessage) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same duplicate console.error issue here - it's logged both here and in showErrorMessage.
| console.error(`Transport error for "${name}":`, error) | ||
| const errorMessage = error instanceof Error ? error.message : `${error}` | ||
| const fullMessage = `Transport error for MCP server "${name}" (SSE): ${errorMessage}` | ||
| console.error(fullMessage) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And again here - duplicate console.error logging.
| const errorMessage = error instanceof Error ? error.message : String(error) | ||
| const fullMessage = `${message}: ${errorMessage}` | ||
| console.error(fullMessage) | ||
| vscode.window.showErrorMessage(fullMessage) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider implementing rate limiting for error notifications. If MCP servers fail repeatedly during reconnection attempts, users might get spammed with toast notifications. Perhaps we could aggregate errors over a time window or implement a cooldown period?
Summary
This PR addresses Issue #7639 by ensuring MCP server errors are properly surfaced to users through VS Code's notification system, rather than only being logged to the console.
Problem
Previously, when MCP server connections failed (invalid URLs, transport errors, etc.), errors were only logged via console.error without any visible UI indication. This resulted in silent failures that appeared as no-ops to users.
Solution
Updated the error handling in McpHub.ts to display errors using vscode.window.showErrorMessage() in addition to console logging:
Changes
Testing
Impact
Users will now receive clear, visible error notifications when:
Fixes #7639
Important
Enhance
McpHub.tsto display MCP server errors in the UI using toast notifications for better user awareness.McpHub.tsnow triggervscode.window.showErrorMessage()for UI notifications.showErrorMessage()updated to include UI notifications.connectToServer()for each transport type now include UI notifications.This description was created by
for 03633d1. You can customize this summary. It will automatically update as commits are pushed.